-
-
Notifications
You must be signed in to change notification settings - Fork 600
ci: Improve coverage #2861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: Improve coverage #2861
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds unit tests (no production code changes) that validate ObjectStateMutations functions correctly handle malicious attribute names (e.g., proto, constructor, prototype) and remove dot-notation array changes when target values are undefined. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2861 +/- ##
===========================================
+ Coverage 99.88% 100.00% +0.11%
===========================================
Files 64 64
Lines 6222 6222
Branches 1473 1489 +16
===========================================
+ Hits 6215 6222 +7
+ Misses 7 0 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/__tests__/ObjectStateMutations-test.js:
- Around line 499-500: The test is passing ParseOps.SetOp objects where
commitServerChanges expects plain attribute values; find the failing data object
in the test (the entry using __proto__: new ParseOps.SetOp({ polluted: 'yes' })
and constructor: new ParseOps.SetOp({ malicious: 'data' })) and replace those
ParseOps.SetOp wrappers with the raw values/objects (e.g., __proto__: {
polluted: 'yes' } or simply polluted: 'yes', and constructor: 'malicious data'
or the plain value you intend) so the payload matches other uses of
commitServerChanges (like the plain { name: 'foo', data: { count: 5 } }
examples).
- Around line 474-490: Update the test to match the actual function being called
and supply the missing third argument: change the description from
"mergeFirstPendingState" to "setPendingOp", and call
ObjectStateMutations.setPendingOp(pendingOps, '__proto__',
pendingOps[0]['__proto__']) (using the third arg from the pendingOps element) so
the function signature (pendingOps, key, op) is satisfied and the test still
verifies Object.prototype is not polluted.
- Around line 438-454: The test is passing ParseOps.SetOp objects to
ObjectStateMutations.setServerData but that function expects plain values;
update the test's attributes to use plain values (e.g., __proto__: { polluted:
'yes' } and constructor: { malicious: 'data' }) before calling
ObjectStateMutations.setServerData(serverData, attributes) so the behavior
matches other tests and the prototype pollution assertions remain valid.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/ObjectStateMutations-test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/ObjectStateMutations-test.js (2)
src/__tests__/SingleInstanceStateController-test.js (1)
ParseOps(23-23)src/__tests__/UniqueInstanceStateController-test.js (1)
ParseOps(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (Node 22, 22.12.0)
- GitHub Check: build (Node 20, 20.19.0)
- GitHub Check: build (Node 24, 24.1.0)
🔇 Additional comments (3)
src/__tests__/ObjectStateMutations-test.js (3)
304-325: LGTM!The test correctly validates that when a dot notation array property is set to
undefined, the property is removed from the array element rather than being set toundefined.
392-413: LGTM!The test properly validates prototype pollution protection in
estimateAttributeby attempting to use malicious attribute names and verifying thatObject.prototyperemains unpolluted.
456-472: LGTM!The test correctly validates prototype pollution protection in
mergeFirstPendingStateusing malicious attribute names with ParseOps objects, which matches the expected usage pattern.
|
🎉 This change has been released in version 8.0.1 |
Pull Request
Issue
Coverage was decreased when merging #2749
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.